Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a info experimental func walk logic #136

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

SungJin1212
Copy link
Contributor

Add an info experimental func to walk logic.

walk.go Outdated
@@ -393,6 +393,9 @@ func (s *PromQLSmith) walkVariadicFunctions(expr *parser.Call) {
case "round":
expr.Args[0] = s.Walk(expr.Func.ArgTypes[0])
expr.Args[1] = &parser.NumberLiteral{Val: float64(s.rnd.Intn(10))}
case "info":
expr.Args[0] = s.Walk(expr.Func.ArgTypes[0])
expr.Args[1] = s.Walk(expr.Func.ArgTypes[1])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second argument has to be a vector selector.
If you do s.Walk(<vector-type>) it will generate an expression which has a result value type to be vector but it can be a different expression like function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can have a dedicated walkInfo function. The second parameter can be either skipped or kept based on rand.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yeya24
Thanks. I have fixed it.

@SungJin1212 SungJin1212 changed the title Add a info experimental func to walk logic Add a info experimental func walk logic Nov 19, 2024
@SungJin1212 SungJin1212 requested a review from yeya24 November 20, 2024 02:55
walk_test.go Outdated
p.walkInfo(expr)
require.Equal(t, parser.ValueTypeVector, expr.Args[0].Type())
if len(expr.Args) == 2 {
require.Equal(t, parser.ValueTypeVector, expr.Args[1].Type())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expr.Args[1].Type() only makes sure the response type is vector.
For this funciton, we need to make sure that expr.Args[1] itself is a vector selector.

Copy link
Contributor Author

@SungJin1212 SungJin1212 Nov 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot.
I'm not familiar with the term.
Is the vector selector like a matcher which can produce a vector?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right. What you did is exactly what I want as this is what upstream Promql engine uses

_, ok := expr.Args[1].(*parser.VectorSelector)

@yeya24
Copy link
Contributor

yeya24 commented Nov 20, 2024

We probably need to bump golang-ci lint. I will merge this PR first and bump its version in another PR

@yeya24 yeya24 merged commit efd4618 into cortexproject:main Nov 20, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants